ServiceContext #19

anjuthomas · 2018-05-18T01:48:54Z

No description provided.

haamel · 2018-05-22T07:42:20Z

src/serviceContext.js

+const assert = require('chai').assert;
+
+/**
+ * @fileoverview This class keeps information that a client needs to be


I don't think that the @fileoverview is needed for this file, as it only contains a single class. Instead I would suggest to make this the class comment.

haamel · 2018-05-22T07:43:52Z

src/serviceContext.js

+/**
+ * serviceContext
+ * @class
+ * @constructor


The annotations @Class and @constructor are redundant here.

have removed the @constructor to avoid redundancy

haamel · 2018-05-22T07:47:32Z

src/serviceContext.js

+ */
+class ServiceContext {
+  /**
+   * This class keeps information that a client needs to be able to talk


Remove the description, as it's the same as above and only keep the @param annotations.

haamel · 2018-05-22T07:51:17Z

src/serviceContext.js

+      } else if (attr === 'requests_dir') {
+        this.request_dir = this.config[attr] || defaultVal;
+      }
+    };


Remove the semicolon.

haamel · 2018-05-22T07:51:33Z

src/serviceContext.js

+      } else if (attr === 'provider_info') {
+        this.provider_info = this.config[attr] || defaultVal;
+      }
+    };


Remove the semicolon.

haamel · 2018-05-22T07:52:37Z

src/serviceContext.js

+    try {
+      this.callback = this.config['callback'];
+    } catch (err) {
+      this.callback = {}


Missing semicolon at EOL.

src/serviceContext.js

@@ -0,0 +1,177 @@
+const State = require('./state.js').State;


haamel · 2018-05-22T09:38:11Z

src/serviceContext.js

+const State = require('./state.js').State;
+const crypto = require('crypto');
+const KeyJar = require('../nodeOIDCMsg/src/oicMsg/keystore/KeyJar').KeyJar;
+const assert = require('chai').assert;


Unused. Remove?

haamel

Some more comments for now, and some general questions.

Also, could you please spend some time on the tests? I haven't made any comments on them yet, but I find some tests, e.g. 'set and get client secret' or 'set and get client id' quite pointless, as these set a member variable directly and immediately afterwards assert that this member was set to the same value. Some more meaningful unit test would be great.

Thanks a lot.

haamel · 2018-05-23T07:26:27Z

src/serviceContext.js

+    let serviceContext = ['client_id', 'issuer', 'client_secret', 'base_url', 'requests_dir'];
+    let defaultVal = '';
+
+    if (params){


{style} Add a whitespace before '{'

haamel · 2018-05-23T07:38:08Z

src/serviceContext.js

+const KeyJar = require('../nodeOIDCMsg/src/oicMsg/keystore/KeyJar').KeyJar;
+
+/** 
+ * ServiceContext


Remove this line as it does not give additional value.

haamel · 2018-05-23T08:00:56Z

src/serviceContext.js

+ * to a server. Some of this information comes from configuration and some
+ * from dynamic provider info discovery or client registration.
+ * But information is also picked up during the conversation with a server.
+ * @class 


No need for this annotation as the 'class' keyword is already used below (line 12) when defining the ServiceContext.

haamel · 2018-05-23T08:03:59Z

src/serviceContext.js

+
+  /**
+   *  Need to generate a redirect_uri path that is unique for a OP/RP combo
+      This is to counter the mix-up attack.


{style} Please put a '*' in front of this comment line to make it clear that it belongs to the block comment.

haamel · 2018-05-23T08:06:39Z

src/serviceContext.js

+
+  setClientSecret(val) {
+    if (!val) {
+      this.client_secret;


I guess you wanted to have the default value set when there is no 'val'. So, this should be:
this.client_secret = '';

haamel · 2018-05-23T11:39:46Z

src/serviceContext.js

+    keyjar = keyjar || null;
+    config = config || null;
+    this.keyjar = keyjar || new KeyJar();
+    this.providerInfo = {};


I noticed that several member variables are defined with similar naming, e.g.:

providerInfo vs. provider_info,

baseUrl vs. base_url,

requestDir vs. requests_dir,

cId vs. client_id,

cSecret vs. clientSecret vs. client_secret,

clientPreferences vs. client_prefs

Is this intentional?

The providerInfo and provider_info is intentional. One refers to the class object and the other refers to config attr. All other names are unintentional and I have fixed it. Good catch ;)

haamel · 2018-05-23T11:49:24Z

src/serviceContext.js

+      }
+    }
+
+    for (let i = 0; i < serviceContext.length; i++) {


Is there a reason you are doing this through a loop? I find it quite hard to read and would rather prefer something like:

this.client_id = this.config['client_id'] || defaultVal; this.issuer = this.config['issuer'] || defaultVal; ...

This would also have the benefit that you do not have to initialize the members with their default value, as done above. If that is the intention, due to the naming inconsistencies it is somewhat hard to tell ;)

Fixed. Removed the for loop.

haamel · 2018-05-23T11:55:48Z

src/serviceContext.js

@@ -0,0 +1,161 @@
+const crypto = require('crypto');
+const KeyJar = require('../nodeOIDCMsg/src/oicMsg/keystore/KeyJar').KeyJar;


The module location will change once you're done with the nodeOIDCMsg repo, right?
If so, could you already require the correct and to be expected location of the module here?

Why would the module location change?

Sorry, this wasn't too clear. What I meant is whether nodeOIDCMsg becomes a dependency of the nodeOIDCService at some point that is listed in the dependency section of the package.json file? If that's the case the path in this in this require statement would slightly change.

On my todo list

haamel · 2018-05-23T12:12:47Z

src/serviceContext.js

+   */
+  constructor(keyjar, config, params) {
+    this.clientSecret = [this.getClientSecret, this.setClientSecret];
+    keyjar = keyjar || null;


Can be removed as it is pointless to initialize keyjar when you initialize this.keyjar properly just 2 lines later.

haamel · 2018-05-23T12:13:15Z

src/serviceContext.js

+  constructor(keyjar, config, params) {
+    this.clientSecret = [this.getClientSecret, this.setClientSecret];
+    keyjar = keyjar || null;
+    config = config || null;


Remove. this.config will be initialized properly in line 26 regardless of this line.

haamel · 2018-05-24T10:00:48Z

src/serviceContext.js

+    return this;
+  }
+
+  getclient_secret() {


Was this change from getClientSecret to getclient_secret intentional?

haamel · 2018-05-24T10:01:47Z

src/serviceContext.js

+    return this.client_secret;
+  }
+
+  setclient_secret(val) {


As above. I think it wasn't your intention to also change this function name?

haamel · 2018-05-24T10:28:20Z

src/serviceContext.js

+   * corresponding file on the local filesystem would be 'jwks_uri'.
+   * Relative to the directory from which the RP instance is run.
+   * 
+   * @param {*} webName 


Could this be of type 'string' rather than '*'?

haamel · 2018-05-24T10:30:46Z

src/serviceContext.js

+   * 
+   * As an example if the base_url is 'https://example.com' and a jwks_uri
+   * is 'https://example.com/jwks_uri.json' then the filename of the
+   * corresponding file on the local filesystem would be 'jwks_uri'.


I think the comment is not correct here. To match with the given example it should state:
"corresponding file on the local filesystem would be 'jwks_uri.json"

haamel · 2018-05-24T10:39:29Z

src/serviceContext.js

+    if (name.startsWith('/')) {
+      return name.substring(1, name.length);
+    } else {
+      let splitName = name.split('/');


Please add more unit tests for this function to cover all the various cases to determine the local filename.

Also cover the case, as mentioned above, when the webName doesn't start with the base_url.

On my TODO list

haamel · 2018-05-24T12:02:35Z

src/serviceContext.js

+      this.redirectUris = [null];
+    }
+
+    try {


This could be simplified to this.callback = this.config['callback'] || {};.

haamel · 2018-05-24T12:03:54Z

src/serviceContext.js

+    }
+
+    if (config && Object.keys(config).indexOf('keydefs') !== -1) {
+      this.keyjar = this.buildKeyJar(config['keydefs'], this.keyjar)[1];


this.buildKeyJar is not defined in this class.

haamel · 2018-05-24T12:05:25Z

src/serviceContext.js

+      if (this.keyjar == null) {
+        this.keyjar = new KeyJar();
+      }
+      this.keyjar.addSymmetric('', val.toString());


Add a unit test that verifies that a symmetric key is added to the keyjar.

On my TODO list

haamel · 2018-05-24T12:21:25Z

test/serviceContext-test.js

+  }
+}
+
+describe('', function() {


What is the purpose of the tests within this describe? It's testing functionality that is not provided by the ServiceContext class.

Since these methods are only for ServiceContext purposes, i thought there is no better place to put these tests..

It's not just about the tests, but mainly about the functionality they are testing. signEncAlgs and verifyAlgSupport are defined in this test class either.

What is the general plan for these methods? If they are used by other classes at some point they should certainly not live here in this test class file. They should rather live in the ServiceContext class or maybe some helper or some other class depending on its further usage.

It probably makes sense to send me more PRs in parallel, so that I get a better understanding of the system and its structure. Would that be possible?

Fixed. Added methods to ServiceContext. Also, I have sent another PR

haamel · 2018-05-24T12:46:16Z

test/serviceContext-test.js

+      } else if (attr === 'base_url') {
+        assert.deepEqual(ci.base_url, config[attr]);
+      } else if (attr === 'requests_dir') {
+        assert.deepEqual(ci.base_url, config[attr]);


Please do not use this kind of "for loop combined with if else if construct" ever again in unit tests. First of all it's really hard to read and furthermore it's error prone.

Like in this the tests pass even though they shouldn't as you deepEqual ci.base_url against config['requests_dir'] which should return false based on the config used and defined above. The root cause here is that this code path is never executed, because the for statement in line 269 is wrong. The for statement uses the keys array (Object.keys(config)) rather than the length of the keys array (Object.keys(config).length) to determine whether should run or not, and apparently is does not run. ;)

Therefore always assert things that should be there, and if things shouldn't be there assert that. This will make your tests more clear, shorter and bullet proof. Or we'll end up having tests for tests ;)

Good catch! Fixed :)

haamel · 2018-06-05T11:09:07Z

test/serviceContext-test.js

+  let ci;
+  beforeEach(function() {
+    config = {
+      'client_id': 'client_id',


I would recommend to not use the key name as its value. This could easily lead to confusion and unnoticed failures.

haamel · 2018-06-05T11:55:47Z

test/serviceContext-test.js

+
+  it('client info init', function() {
+    assert.isNotNull(ci);
+    if (Object.keys(config).indexOf('client_id') > -1) {


Once again: assert things that should be there, and if things shouldn't be there assert that!

First of all, this if (...) {...} else if {... block will only test one of the five key/value pairs. That's probably not what you meant to test.

If you wanted to test whether the ServiceContext.config is equal to the provided one in the constructor, then you could simply do the following instead of the if block:

assert.deepEqual(ci.config, config);

If you also wanted to check whether the other members, like client_id, are initialized correctly as well you should do the following in addition:

assert.strictEqual(ci.client_id, 'client_id'); assert.strictEqual(ci.issuer, 'issuer'); assert.strictEqual(ci.client_secret, 'client_secret'); assert.strictEqual(ci.base_url, 'https://example.com'); assert.strictEqual(ci.request_dir, 'requests');

Also, client_secret is quite special as it should be accessed through getClientSecret or setClientSecret rather than ci.client_secret, right?
Therefore you could also use the following:

assert.strictEqual(ci.getClientSecret(), 'client_secret');

Nevertheless, setClientSecret and getClientSecret should have their own tests.

Overall this will make your tests more clear, shorter and bullet proof.

haamel · 2018-06-05T12:17:55Z

src/serviceContext.js

+
+    this.client_id = this.config['client_id'] || defaultVal;
+    this.issuer = this.config['issuer'] || defaultVal;
+    this.client_secret = this.config['client_secret'] || defaultVal;


Remove this line.

haamel · 2018-06-06T04:51:39Z

src/serviceContext.js

+   * @param {string} typ: 'id_token', 'userinfo' or 'request_object'
+   */
+  signEncAlgs(typ) {
+    let serviceContext = this;


Remove this and replace all occurrences of serviceContext. in this function with this..

haamel · 2018-06-06T04:53:22Z

src/serviceContext.js

+    for (let i = 0; i < Object.keys(ATTRMAP[typ]).length; i++) {
+        let key = Object.keys(ATTRMAP[typ])[i];
+        let val = ATTRMAP[typ][key];
+        if (serviceContext.registrationResponse && serviceContext.registrationResponse[val]){


{style} Missing whitespace between ) and {.

haamel · 2018-06-06T12:19:14Z

src/serviceContext.js

+   *        - id_token
+   *        - request_object
+   *        - token_endpoint_auth
+   * @param {string} typ Type of alg


Rename this parameter into type. It's more intuitive.

haamel · 2018-06-06T12:24:52Z

src/serviceContext.js

+   */
+  verifyAlgSupport(alg, usage, typ) {
+    let serviceContext = this;
+    let supported = serviceContext.providerInfo[usage + '_' + typ + '_values_supported'];


What if providerInfo is not set or the constructed key is not present in providerInfo?

haamel · 2018-06-06T13:37:52Z

src/serviceContext.js

+    this.client_id = this.config['client_id'] || defaultVal;
+    this.issuer = this.config['issuer'] || defaultVal;
+    this.client_secret = this.config['client_secret'] || defaultVal;
+    this.setClientSecret(this.client_secret);    


Change to

this.setClientSecret(this.config['client_secret']);

haamel · 2018-06-06T14:10:16Z

test/serviceContext-test.js

+  });
+
+  it('client info init', function() {
+    assert.deepEqual(ci.clientSecret, 'supersecret');


What is the intention of this test?
It should literally always pass, unless you decide using the new ES6 set/get class syntax. Then it might fail depending on your implementation.

Also it is not clientSecret, but client_secret.

haamel · 2018-06-06T14:11:00Z

test/serviceContext-test.js

+  });
+
+  it('client info init clientId', function() {
+    assert.deepEqual(ci.clientId, 'myself');


What is the intention of this test?
It should literally always pass, unless you decide using the new ES6 set/get class syntax. Then it might fail depending on your implementation.

Also it is not clientId, but client_id.

anjuthomas added 2 commits May 17, 2018 18:41

ServiceContext files

13f92ec

ServiceContext files

db46883

anjuthomas requested a review from haamel May 18, 2018 05:08

haamel reviewed May 22, 2018

View reviewed changes

src/serviceContext.js Outdated

@@ -0,0 +1,177 @@

const State = require('./state.js').State;

This comment was marked as resolved.

Sign in to view

This comment was marked as resolved.

Sign in to view

haamel reviewed May 22, 2018

View reviewed changes

Revised ServiceContext

e89cd37

haamel reviewed May 23, 2018

View reviewed changes

Revised ServiceContext 2

1d06814

haamel reviewed May 24, 2018

View reviewed changes

anjuthomas added 2 commits May 28, 2018 23:01

Revised ServiceContext 3

62f0214

Revised ServiceContext 4

91504ed

haamel reviewed Jun 6, 2018

View reviewed changes

		@@ -0,0 +1,161 @@
		const crypto = require('crypto');
		const KeyJar = require('../nodeOIDCMsg/src/oicMsg/keystore/KeyJar').KeyJar;

ServiceContext #19

Are you sure you want to change the base?

ServiceContext #19

Uh oh!

Conversation

anjuthomas commented May 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anjuthomas May 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

haamel May 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haamel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anjuthomas May 22, 2018 •

edited

Loading

haamel May 22, 2018 •

edited

Loading